Walkthrough

Adds a persistent, runtime-configurable Settings domain with Get/Update/Clear admin APIs, DB migrations and repo implementations (badger/sqlite/postgres), wiring to load/apply settings at startup and runtime, plus docs, Makefile, env and e2e test updates to use admin-managed settings.

Changes

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant GRPC as "gRPC Handler"
    participant AdminApp as "Admin Service"
    participant RepoMgr as "Repo Manager"
    participant SettingsRepo as "Settings Repo"
    participant AppSvc as "App Service"

    Client->>GRPC: UpdateSettings(request)
    GRPC->>AdminApp: UpdateSettings(settings)
    AdminApp->>AdminApp: validate & set UpdatedAt
    AdminApp->>RepoMgr: Settings()
    RepoMgr-->>AdminApp: SettingsRepository
    AdminApp->>SettingsRepo: Upsert(settings)
    SettingsRepo->>SettingsRepo: persist to storage
    SettingsRepo-->>AdminApp: OK
    AdminApp->>AppSvc: onSettingsUpdated / UpdateSettings(settings)
    AppSvc->>AppSvc: recalc derived values & tapscript
    AppSvc-->>AdminApp: OK
    AdminApp-->>GRPC: UpdateSettingsResponse
    GRPC-->>Client: Response

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Fix redis implementation of current round #813 — touches internal/core/application/service.go; likely overlaps on service-level update logic and tapscript handling.
• Proto renaming #737 — modifies configuration/startup surfaces and env-var handling; related to the config/env removals and Makefile/README changes here.

Suggested reviewers

• altafan
• louisinger

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

🔍 Arkana PR Review — arkd#939: Settings Domain with DB Persistence

Scope: Moves 16 runtime-tunable config values (ban threshold/duration, exit delays, VTXO tree expiry, round participant limits, amount limits, etc.) from environment variables into a DB-persisted Settings domain with admin CRUD API. Touches 38 files, +2413/−199 lines.

Overall: Well-structured domain model following the existing repository pattern. Clean separation across Postgres, SQLite, and Badger backends with proper migrations. Good test coverage. A few concerns below, some security-relevant.

🔴 Security: No Input Validation on UpdateSettings

internal/interface/grpc/handlers/adminservice.go — UpdateSettings passes proto values straight through without any validation. Since proto3 uses zero values for unset fields, a partial update attempt (e.g., only setting ban_threshold) would zero out all other fields including critical ones like unilateral_exit_delay, boarding_exit_delay, and vtxo_tree_expiry.

Risks:

• Setting unilateral_exit_delay = 0 or boarding_exit_delay = 0 would break the unilateral exit guarantee — users could lose the ability to safely exit.
• Setting vtxo_tree_expiry = 0 would create immediately-expired VTXO trees.
• Setting max_tx_weight = 0 would reject all transactions.
• Negative or nonsensical values for delays/amounts aren't rejected.

Suggestion: Add a Validate() method on domain.Settings that enforces minimum safe values, and call it in UpdateSettings before persisting. At minimum: exit delays > 0, vtxo_tree_expiry > unilateral_exit_delay, boarding_exit_delay > vtxo_tree_expiry, etc. The existing validation in config.go:Validate() already checks some of these constraints — factor those checks into a shared function.

Also consider using proto optional fields or a field mask to distinguish "set to zero" from "not provided", enabling safe partial updates.

🟡 Bug: ClearSettings Doesn't Reset Running Service

internal/core/application/admin.go — ClearSettings calls repo.Clear() and onInfoChange() but does not call onSettingsUpdated or reset the in-memory service settings to defaults. The running arkd keeps the previously loaded settings until restart. This is inconsistent with the "live propagation" behavior of UpdateSettings.

func (a *adminService) ClearSettings(ctx context.Context) error {
    if err := a.repoManager.Settings().Clear(ctx); err != nil {
        return err
    }
    // Missing: reset service to default settings
    a.onInfoChange()
    return nil
}

Should either: (1) re-seed defaults and call onSettingsUpdated with them, or (2) document that ClearSettings requires a restart and update the API docs accordingly.

🟡 Duplicated Locktime Logic

internal/core/application/service.go:toRelativeLocktime() duplicates the block-vs-time threshold logic from config.go:determineLocktimeType() with a hardcoded minAllowedSequence = 512. If this threshold ever changes in one place but not the other, settings loaded at startup vs settings updated via API would interpret the same values differently.

Suggestion: Extract the shared logic into a single function in domain or a shared package.

🟢 Positives

• Clean architecture: Domain model → repository interface → 3 backend implementations follows the existing patterns well.
• Live propagation: UpdateSettings callback recalculates derived values (dust amounts, checkpoint tapscript) correctly in service.go:UpdateSettings().
• Good test coverage: Unit tests for admin service, integration tests for all 3 DB backends, e2e tests updated to use the new API.
• Proper auth gating: New endpoints use EntityManager read/write permissions consistent with other admin endpoints.
• Migration files: Present for both Postgres and SQLite with proper up/down scripts.
• README updated: Documents the new admin settings API with defaults table.

Minor Notes

• Postgres UpsertSettings always inserts with id=0 (Go zero value). Works because GENERATED BY DEFAULT AS IDENTITY allows explicit values, but it's implicit — a comment would help.
• The NewAdminService signature now requires an onSettingsUpdated callback — breaking change for any external consumers of that constructor.
• settings table schema uses BIGINT NOT NULL DEFAULT 0 for all fields. Since -1 is a valid sentinel for amount limits ("unset"/"dust"), the DEFAULT 0 in the schema doesn't match the domain semantics. Not a functional bug (the app always provides explicit values), but could be confusing.

Verdict: The architectural approach is solid and the implementation is thorough. The missing input validation on UpdateSettings is the most critical item — it's a live admin API that directly controls protocol-security parameters (exit delays, VTXO expiry). Would recommend addressing the validation gap before merge.

addressed the comments from @arkanaai (#939 (comment)) in: 5271785

Thanks for addressing the feedback, the changes look good overall. A few notes on the commit:

✅ Locktime deduplication — clean, / is the right home.

✅ ClearSettings — now correctly re-seeds defaults and calls onSettingsUpdated. The test rename to "resets settings to defaults" reflects the actual behaviour. Good.

✅ Validate() — covers the critical cases (zero exit delays, min/max participants ordering, public ≥ unilateral). Two small follow-up items:

1. Missing ordering constraint: VtxoTreeExpiry isn't checked against UnilateralExitDelay. Trees should expire before users need to exit, so probably want VtxoTreeExpiry > UnilateralExitDelay (or at least document why it's intentionally omitted).

2. String-based error detection in the gRPC handler:

   This is fragile — if the error message ever changes, validation errors silently become codes.Internal instead of codes.InvalidArgument. Consider using a sentinel error or errors.As with a typed validation error instead.

Neither is a blocker, but worth a quick fix before merge.

🔍 PR Review: Settings Domain with DB Persistence and Admin CRUD API

Reviewer: Arkana (automated)
Scope: +2514/-221 across 38 files — migrates operator settings from env vars to DB-persisted Settings domain with admin API

Summary

This PR moves ~15 operator-configurable parameters (exit delays, VTXO tree expiry, round limits, ban thresholds, amount bounds, etc.) out of environment variables into a new Settings domain entity backed by DB persistence (badger/sqlite/postgres). It adds three admin API endpoints (GET/POST /v1/admin/settings, POST /v1/admin/settings/clear) and enables hot-reload of these parameters into the running service without restart.

✅ What looks good

• Clean domain modeling: domain.Settings with Validate() and SettingsRepository interface is well-structured.
• Multi-backend support: Proper implementations for badger, sqlite, and postgres with migrations.
• Default seeding on first startup: loadSettings() in config seeds defaults if none exist — good for fresh deployments.
• Admin permissions properly gated: Macaroon permissions correctly map to EntityManager read/write.
• Comprehensive tests: Unit tests for admin service settings CRUD and integration tests across all DB backends.
• E2E tests updated: setupArkd() now calls updateSettings() to configure test parameters via API.
• ClearSettings resets to defaults rather than leaving the system in an undefined state.

⚠️ Concerns

1. Race condition in service.UpdateSettings() (medium severity)

internal/core/application/service.go — UpdateSettings() mutates ~15 fields on the service struct (s.banDuration, s.batchExpiry, s.unilateralExitDelay, etc.) without any synchronization. If a round is being processed concurrently, it could read a mix of old and new values mid-update. Consider holding the service mutex (or adding one if there isnt one) during the settings update, or using an atomic swap of a settings snapshot.

2. No guard against changing exit delays for live VTXOs (high severity — protocol correctness)

Changing unilateral_exit_delay, boarding_exit_delay, or vtxo_tree_expiry at runtime affects new rounds, but existing VTXOs were committed with the old values baked into their scripts. The system should either:

• Document clearly that these changes only affect future rounds (and ensure no code path applies new delays to existing VTXOs), or
• Reject changes to exit delays while there are unsettled VTXOs, or
• At minimum, log a prominent warning when these critical parameters change.

This is the most security-sensitive aspect of this PR. An operator accidentally reducing unilateral_exit_delay mid-operation could create confusion about exit guarantees.

3. Postgres upsert ID defaults to 0 implicitly

In postgres/settings_repo.go, UpsertSettingsParams.ID is never set (defaults to 0). This works as a singleton pattern via ON CONFLICT(id), but its implicit. Consider either:

• Using a constant settingsSingletonID = 1 and setting it explicitly, or
• Adding a comment explaining the singleton pattern.

The same applies to sqlite/settings_repo.go.

4. Validate() could be stricter

domain/settings.go Validate() doesnt check:

• VtxoTreeExpiry must be reasonable (e.g., > UnilateralExitDelay)
• CheckpointExitDelay relationship to other delays
• MaxTxWeight must be > 0
• BanThreshold must be > 0

Some of these are enforced later in config.Validate(), but since UpdateSettings calls settings.Validate() directly, the API endpoint could accept values that config.Validate() would reject.

5. updated_at stored as unix timestamp loses sub-second precision

The postgres/sqlite schemas store updated_at as BIGINT (unix seconds). The badger store preserves Go time.Time directly. This means time.Unix(settings.UpdatedAt, 0) in the SQL repos truncates to seconds. Not a bug, but a minor inconsistency worth noting.

6. Settings schema uses BIGINT GENERATED BY DEFAULT AS IDENTITY (postgres)

This allows manual ID insertion (the 0 from point 3), but also allows auto-increment if multiple rows are inserted without explicit ID. Since the code always passes 0, this works, but GENERATED ALWAYS would be safer to prevent accidental multi-row scenarios. Or drop auto-generation entirely since this is a singleton table.

📝 Minor notes

• Docker compose and dev env files cleanly remove the old env vars — good cleanup.
• The Makefile simulation simplification (removing MIN/MAX params) is a nice side effect.
• Proto and OpenAPI specs are consistent with the implementation.
• minAllowedSequence correctly moved to domain.MinAllowedSequence as a public constant.

Verdict

Architecturally sound refactor that improves operability significantly. The main concerns are around race safety during hot-reload and protocol correctness guarantees when exit delays change for live VTXOs. These should be addressed before merge.

🔍 Follow-up Review — New Commits (da53ecf, a61a9f1)

Thanks for addressing the feedback, @bitcoin-coder-bob. This is a solid round of fixes.

✅ Issues Resolved

1. Race condition — settingsMu mutex now guards both UpdateSettings and ClearSettings. The comment on service.UpdateSettings correctly documents the caller-holds-lock contract. Fixed.

2. ClearSettings inconsistency — Now properly resets roundMinParticipantsCount and roundMaxParticipantsCount on the admin service after clearing DB. Fixed.

3. Apply-before-persist ordering — Settings are now applied to the live service first, then persisted. If onSettingsUpdated fails, the DB retains the previous valid settings. Good defensive pattern. Fixed.

4. MaxTxWeight validation — Validate() now rejects MaxTxWeight <= 0. Fixed.

5. Badger retry respects context — select on ctx.Done() prevents infinite retry loops if the context is cancelled. Fixed.

6. README & comments — Endpoint table, ToRelativeLocktime godoc, and inline comments all improve clarity.

🟡 Remaining Concern (from prior review, still open)

No guard against changing exit delays for live VTXOs. If an operator reduces unilateral_exit_delay or boarding_exit_delay while VTXOs with longer timelocks are in-flight, those VTXOs were committed with the old delay baked into their scripts. The DB settings would diverge from what's actually enforced on-chain. This is a protocol correctness issue rather than a code bug — the validation layer doesn't have visibility into existing VTXO commitments.

This doesn't need to block merge, but a follow-up issue to add "minimum floor" validation (or at least a warning log when reducing exit delays) would be valuable.

🟢 Overall

The mutex, apply-before-persist, and validation additions are all the right calls. This is in much better shape. No new blocking issues.

addressed @arkanaai comment for string based error detction (#939 (comment)) in: 2802d62

however for VtxoTreeExpiry > UnilateralExitDelay while we can normalize the comparison using RelativeLocktime.Seconds(), the block scheduler defaults would fail it (vtxoTreeExpiry=20 blocks ≈ 12,000s vs
unilateralExitDelay=86,400s), and config.go:Validate() doesn't enforce this either. Skipping for now to avoid breaking block scheduler configs. The bit inside defaultSettings() in config.go:

	if c.SchedulerType == "block" {
		vtxoTreeExpiry = 20      // 20 blocks
		checkpointExitDelay = 10 // 10 blocks
	}

this is the case that would break things, since now vtxoTreeExpiry is now not going to exceed UnilaterExitDelay. The Validate() method on Settings doesn't know which scheduler is in use. So it would either need the scheduler type passed in, or the check would have to be done at the config level instead. Either way it would break block scheduler configs as they exist today.

Makes sense — the block scheduler case is a legitimate exception and you're right that Validate() doesn't have the scheduler context to distinguish them. Adding the check there would either false-positive on block configs or require threading scheduler type through, neither of which is worth it here.

Skipping the VtxoTreeExpiry > UnilateralExitDelay check is reasonable. If it's worth documenting the invariant somewhere for future reference, a comment in Validate() or defaultSettings() noting "block scheduler configs intentionally have vtxoTreeExpiry < unilateralExitDelay" would help the next person not re-raise this.

Otherwise this looks good to me — both original blockers are resolved.

🔍 Arkana Review — Follow-up (commit 2802d62)

Scope: Single commit since last review — introduces ErrInvalidSettings typed error.

Changes

1. domain/settings.go — New ErrInvalidSettings struct with Reason field. All Validate() returns now use this typed error instead of raw fmt.Errorf.
2. application/admin.go — UpdateSettings now returns the validation error directly (no wrapping), preserving the error type for callers.
3. grpc/handlers/adminservice.go — Replaces fragile strings.Contains(err.Error(), "invalid settings:") with proper errors.As(err, &validationErr). This is the right pattern — type assertions are stable across refactors, string matching is not.

Assessment

✅ Clean improvement. The typed error approach:

• Makes the gRPC handler resilient to message wording changes
• Allows SDK consumers to programmatically distinguish validation failures
• Follows Go error handling best practices (errors.As over string inspection)

No security, correctness, or cross-repo concerns with this change.

Prior Review Items Status

All previously flagged issues from earlier commits remain addressed. The one non-blocking concern from the prior review still stands: no guard against reducing exit delays for live VTXOs with existing timelocks. This is a protocol-level design decision, not a code bug.

No further action needed on this commit.

for the commnets by @arkanaai in: #939 (comment) they were addressed in commit: 557ccad

Thanks. The changes in 557ccad look good:

• Upsert comment (postgres + sqlite) — clearly documents the singleton id=0 pattern. Exactly what was needed.
• Exit delay warnings in UpdateSettings() — the three log.Warnf calls on change cover the critical parameters. Good that they're logged before the values are applied, so the log captures old→new correctly.

One small thing: publicUnilateralExitDelay and checkpointExitDelay changes aren't warned on, only the three main ones. Probably fine since those are less security-critical, but worth being consistent if you want complete auditability.

No blockers from me on this PR.

🔍 Arkana Review — Commit 557ccad6 (warning logs + comments)

Scope: service.go — warning logs on exit delay changes; postgres/settings_repo.go & sqlite/settings_repo.go — doc comments on Upsert singleton pattern.

✅ Looks good

• Exit delay change warnings — Exactly what was needed. The log.Warnf calls clearly communicate that changes only affect future rounds, not live VTXOs. Good operator visibility for a parameter that has protocol-level implications.
• Clean refactor — Computing newUnilateral, newBoarding, newExpiry before comparison avoids redundant ToRelativeLocktime calls and makes the diff-then-assign flow readable.
• Singleton key documentation — Clarifying the ON CONFLICT(id) / Go zero-value pattern removes a potential maintenance trap.

🟢 No concerns

This commit is additive (logging + comments only), no behavioral changes. LGTM.

I summoned the AI who actually had some valid concerns:

Blocking / High

  1. The "singleton" is enforced by nobody, and three code paths disagree on what "the row" is.
  - Get → SelectLatestSettings = ORDER BY updated_at DESC LIMIT 1 (postgres/sqlite query.sql)
  - Upsert → INSERT ... ON CONFLICT(id) DO UPDATE with id defaulting to 0 (Go never sets it)
  - InsertSettingsHistory → hardcodes WHERE s.id = 0
  
  The schema (id GENERATED BY DEFAULT AS IDENTITY / AUTOINCREMENT) places no CHECK/unique guarantee that only one row exists. If a second row ever lands (a stray insert, a future code path that sets id,
  a botched restore), Get follows updated_at while upsert/history follow id=0, and they silently diverge. updated_at is second-granularity (timeToUnix uses .Unix()), so the ORDER BY tiebreak is also
  non-deterministic within a second. This is exactly the failure class a CHECK (id = 1) (or id = 0) singleton constraint prevents, and matches the "restore the singleton CHECK" instinct from earlier.
  Recommendation: enforce one row at the schema level and make all three paths agree on a fixed id (WHERE id = 1), rather than relying on three separate conventions.

  2. Every process restart writes a no-op settings_history row and fires a spurious "settings changed" event.
  Boot path service.go:103-116: resolveMinAmounts is idempotent, but settings.Update({VtxoMinAmount, UtxoMinAmount}) appends to the changelog based purely on pointer-non-nil, never value comparison
  (settings.go:293-308). So the changelog is always ["vtxo_min_amount","utxo_min_amount"] on boot → postgres Upsert sees a non-empty changelog → InsertSettingsHistory runs (settings_repo.go:156-161) → an
  audit row on every restart, even when nothing changed. This directly defeats the stated intent in that same code ("Only record history for actual changes"). It also means the very first history entry
  is a phantom reconciliation of the seed, and any "settings changed" warning logs fire on every boot. Recommendation: gate the write/history on an actual value delta (compare before appending to
  changelog, or only persist if resolveMinAmounts changed something), or compute resolved min-amounts for the cache without persisting.

  3. UpdateSettings is an unserialized read-modify-write, and the cache refresh is a fire-and-forget goroutine whose ordering isn't tied to the DB.
  - adminService.UpdateSettings (admin.go:585-611): Get → settings.Update → Upsert with no lock (confirmed: no mutex in admin.go). Two concurrent admin updates each read the same base and the later
  Upsert clobbers the earlier — classic lost update.
  - postgres/sqlite Upsert ends with go r.dispatch(...) (settings_repo.go:168), and the registered handler (service.go:347-365) does its own unsynchronized cache Get → mutate → Upsert. The cache's
  RWMutex makes each call atomic but not the sequence, and the dispatch goroutines for two updates can run in either order. Net result: the cache can settle on an older value than the DB. DB/cache
  divergence with no reconciliation until restart. Redis doesn't save you here — its Watch wraps an unconditional Set, so it provides no read-check (see nit below).
  
  Settings changes are rare, so the probability is low, but it's real and silent. Recommendation: serialize UpdateSettings (mutex or SELECT ... FOR UPDATE), and make the cache update synchronous/ordered
  relative to the committed write (or refresh the cache from the DB rather than from the in-flight settings value).

  ---
  Medium
  
  4. The UpdateSettings doc comment is not just stale, it's inverted (admin.go:588-592). It describes a removed updateFields mechanism and states "Fields not set in the request default to 0 so callers 
  must populate all fields." The actual behavior is the opposite: pointer-based partial merge, omitted fields are preserved. A maintainer trusting this comment would assume an omitted field zeroes out.
  Fix or delete.

  5. Validation gaps vs. stated intent (settings.go:107-214). MaxSatoshis (line 14) is defined and used nowhere in the repo — the "add satoshi upper bound on amounts" intent never landed. There's no
  vtxo/utxo_max_amount >= min check (you can set max=5, min=100 and it validates), and MaxOpReturnOutputs is unvalidated. Either wire MaxSatoshis + a min/max ordering check, or drop the dead constant.

  6. ClearSettings is a half-wired/dead feature. repo.Clear() (DELETE FROM settings) is implemented in all three backends but called from no non-test code, the permission map references
  /AdminService/ClearSettings (permissions.go:408) for an RPC that doesn't exist (no admin.proto entry, no handler, no app method), and the README documents /v1/admin/settings/clear. Decide: implement
  it, or remove the repo method + permission + README row. Note Clear() is also dangerous given (1): on the current schema it empties the table but the seed only re-runs at boot, so a runtime clear would
  leave Get returning nil.

  7. InsertSettingsHistory ... WHERE s.id = 0 can silently snapshot nothing. If the row's id is ever not 0, the INSERT ... SELECT ... WHERE s.id=0 inserts zero rows: the upsert succeeds, the audit record
  is silently lost. Same root cause as (1).

  8. Inconsistent locking / data race on the handler field. dispatch reads r.updateHandler without updateHandlerMu, which RegisterUpdatesHandler holds when writing (settings_repo.go:42-46, 181-185, same
  in sqlite/badger). Benign in practice (registered once at startup), but it's a genuine race under -race and the mutex is half-used. Use the lock on read, or make it set-once/atomic.

  ---
  Low / nits

  - 9. README (as discussed): "Update settings (full replace)" is wrong (it's partial); the /clear endpoint doesn't exist; and the seed ARKD_* vars were deleted from the env table yet are still
  functional and still used in docker-compose.regtest.yml. They need a "first-boot seed only" note.
  - 10. Terminology drift: domain uses RoundMin/MaxParticipantsCount but the user-facing error strings say "batch min/max participants count" (settings.go:205,209) and the handler uses
  batchMinParticipants. Pick one vocabulary.
  - 11. sqlite AUTOINCREMENT is unnecessary (and slightly costly) for a singleton; AssetTxMaxWeightRatio round-trips float32↔float64 differently across backends (sqlite casts, postgres native). Cosmetic.
  - 12. Redis Upsert (redis/settings.go:36-57) wraps an unconditional Set in Watch/TxPipelined with retry, but never reads the key, so the optimistic-lock machinery buys nothing over a plain Set.
  Harmless, but misleading.
  - 13. inmemory settingsStore.Get returns a zero-value ports.Settings (not an error/nil) when never populated — a footgun if ever read before boot population.